Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

This has build failures that need to be addressed. Looks to be warning due to the usage of an API marked preview.

@tannergooding I added suppression for "CA2252 // This API requires opting into preview features" in source.

I assume there is a good reason why preview features are no longer allowed System.Private.CoreLib when it was previously allowed for "dogfooding preview features", so looking at enabling it for the project seemed to be out of question.

Is there a better way to solve it available to corelib / the dotnet runtime repro?

This API is marked with Preview attribute because it produces suboptimal code. Have you verified that this change actually improves performance?

In general, any change that is a performance optimization needs to come with perf numbers.

This API is marked with Preview attribute because it produces suboptimal code. Have you verified that this change actually improves performance?

It is not optimal, but it is quite good

As I understand it the major issue which prevents general usage in Math.DivRem is that constant detection (and maybe propagation) is not yet done. The generated code is can often be optimal, unless you use to many value tuple calls and it starts to spill to the stack.

As for preview

In general, any change that is a performance optimization needs to come with perf numbers.

Yes, see Uint128 division benchmarks below, the it should give a much better confidence than my original statement about it being at least 10ns faster. (it is almost 20ns or 9 times faster)

This PR

commit 60cc066

(I built the benchmarks as self contained and then copied over all dll's from (runtime\artifacts\bin\coreclr\windows.x64.Release)

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.4.24267.66
  [Host] : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

Job=InProcess  Toolchain=InProcessEmitToolchain  

NET 9 preview 4

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.4.24267.66
  [Host] : .NET 9.0.0 (9.0.24.26619), X64 RyuJIT AVX2

Job=InProcess  Toolchain=InProcessEmitToolchain  

[Params(0, 1)]
public ulong A_high
{
    get => aHi;
    set
    {
        aHi = value;
        _a = new UInt128(aHi, aLo);
    }
}

[Params(0x00006789abcdef0)]
public ulong A_low

{
    get { return aLo; }
    set
    {
        aLo = value;
        _a = new UInt128(aHi, aLo);
    }
}

[Params(0x123456789abcdef1)]
public ulong B
{
    get => (ulong)_b;
    set => _b = new UInt128(0ul, value); 
}

[Benchmark]
public UInt128 DivisionByUInt64()
{
    return _a / B;
}

[Benchmark]
public UInt128 DivisionByUInt128()
{
    return _a / _b;
}

[Benchmark]
public UInt128 DivisionByConstant()
{
    return _a / 7;
}

</details>

I have done quick perf tests:

using System;
using System.Diagnostics;

var sw = new Stopwatch();

for (;;)
{
   sw.Restart();
   UInt128 sum = 0;
   for (UInt128 i = 1; i < 100_000_000; i++) sum += (UInt128)i / (UInt128)3;
   Console.WriteLine(sw.ElapsedMilliseconds);
}

It runs 1.3x slower with this change. Would it be better to apply this optimization only for non-zero higher bits?

Would it be better to apply this optimization only for non-zero higher bits?

I've changed the order so that the new code is only used as a fallback in case of non-zero higher bits.
It seems to improve your example from an average of 147 to 127 for me (plain preview.4 gives around 140)

My best guess is that the performance change comes from PGO based aggressive inlining in which case the standard division can get replaced with mul and shift, but the instrinct will not.

Rerunning CI. Going to finish reviewing this today after CI finishes

@tannergooding I have updated the suppressions for this PR and the decimal PR to handle the introduction of ExperimentalAttribute, so it compiles again.

This looks good. But could you give some updated perf numbers prior to this getting merged?

@tannergooding I reran above benchmarks with rc1 and the last commit of this branc

PR

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4317/23H2/2023Update/SunValley3)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
  [Host] : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX2

Job=InProcess  Toolchain=InProcessEmitToolchain  

RC.1

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4317/23H2/2023Update/SunValley3)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-rc.1.24452.12
  [Host] : .NET 9.0.0 (9.0.24.43107), X64 RyuJIT AVX2

Job=InProcess  Toolchain=InProcessEmitToolchain  

your benchmark program

Running with runtime win-x64 - .NET 9.0.0-rc.1.24431.7
1930
1889
1895
1899
1893

❯ .\dotnet.exe C:\Users\danie\source\repos\BenchmarkApp\bin\Release\net9.0\win-x64\BenchmarkApp.dll
Hello, World!
Running with runtime win-x64 - .NET 10.0.0-dev
1861
1857
1875
1879
1878